Skip to content

Conversation

@Qelxiros
Copy link
Contributor

@Qelxiros Qelxiros commented Sep 30, 2025

closes #147147

The suggestion span is wrong, but I'm not sure how to find the right one. fixed

I'm relatively new to the diagnostics ecosystem, so I'm not sure if span_help is the right choice. span_suggestion_* might be better, but the output from x test looks weird in that case.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 30, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2025

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rust-log-analyzer

This comment has been minimized.

Comment on lines 10 to 13
note: the foreign item type `String` doesn't implement `BuildHasher`
--> $SRC_DIR/alloc/src/string.rs:LL:COL
|
= note: not implement `BuildHasher`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self, this probably needs improvement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Could there be other tests that showcase when this help message could appear? For example when I try to insert, or when I try to create a new value of the HashSet type.

Also I think it might be better to put this under the less crowded tests/ui/hashmap directory.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2025
@fee1-dead
Copy link
Member

Still waiting on other tests.

Also, use [@]rustbot ready if you want to resubmit for review

@Qelxiros
Copy link
Contributor Author

I've added a test for the insert case. It seems pretty representative of most instances of this error, since the HashSet bounds are on methods. If we want to lint (not error) on HashSet construction with a second parameter that doesn't impl BuildHasher (and I'm not convinced we do), that's a separate conversation.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2025
Comment on lines 14 to 21
help: you might have intended to use a HashMap instead
--> $DIR/hashset_generics.rs:8:5
|
LL | #[derive(PartialEq)]
| --------- in this derive macro expansion
...
LL | pub parameters: HashSet<String, String>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use err.help() to avoid highlighting the same thing again?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2025
@bors
Copy link
Collaborator

bors commented Oct 22, 2025

☔ The latest upstream changes (presumably #147957) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Oct 30, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Qelxiros
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2025
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2025
@Qelxiros
Copy link
Contributor Author

Qelxiros commented Nov 5, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2025
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had one last nit, otherwise this is good to go!

View changes since this review

Comment on lines 3085 to 3097
if foreign_preds.iter().any(|&(root_pred, pred)| {
if let ty::PredicateKind::Clause(ty::ClauseKind::Trait(root_pred)) =
root_pred.kind().skip_binder()
&& let Some(root_adt) = root_pred.self_ty().ty_adt_def()
{
self.tcx.is_diagnostic_item(sym::HashSet, root_adt.did())
&& self.tcx.is_diagnostic_item(sym::BuildHasher, pred.def_id())
} else {
false
}
}) {
err.help("you might have intended to use a HashMap instead");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to extract this to a helper function that deduplicates the logic between this one and the one above, maybe named something like "suggest_hashmap_on_unsatisfied_hashset_buildhasher"

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2025
@Qelxiros
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 18, 2025
@fee1-dead
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 18, 2025

📌 Commit 754a82d has been approved by fee1-dead

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2025
bors added a commit that referenced this pull request Nov 19, 2025
Rollup of 7 pull requests

Successful merges:

 - #147171 (recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher)
 - #147421 (Add check if span is from macro expansion)
 - #147521 (Make SIMD intrinsics available in `const`-contexts)
 - #148201 (Start documenting autodiff activities)
 - #148797 (feat: Add `bit_width` for unsigned `NonZero<T>`)
 - #148798 (Match <OsString as Debug>::fmt to that of str)
 - #149082 (autodiff: update formating, improve examples for the unstable-book)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 847c422 into rust-lang:main Nov 19, 2025
11 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Nov 19, 2025
rust-timer added a commit that referenced this pull request Nov 19, 2025
Rollup merge of #147171 - Qelxiros:hashmap_diag, r=fee1-dead

recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher

closes #147147

~The suggestion span is wrong, but I'm not sure how to find the right one.~ fixed

I'm relatively new to the diagnostics ecosystem, so I'm not sure if `span_help` is the right choice. `span_suggestion_*` might be better, but the output from `x test` looks weird in that case.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Nov 20, 2025
Rollup of 7 pull requests

Successful merges:

 - rust-lang/rust#147171 (recommend using a HashMap if a HashSet's second generic parameter doesn't implement BuildHasher)
 - rust-lang/rust#147421 (Add check if span is from macro expansion)
 - rust-lang/rust#147521 (Make SIMD intrinsics available in `const`-contexts)
 - rust-lang/rust#148201 (Start documenting autodiff activities)
 - rust-lang/rust#148797 (feat: Add `bit_width` for unsigned `NonZero<T>`)
 - rust-lang/rust#148798 (Match <OsString as Debug>::fmt to that of str)
 - rust-lang/rust#149082 (autodiff: update formating, improve examples for the unstable-book)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider suggesting using HashMap<String, String> instead of HashSet<String, String> when PartialEq derive macro is used

5 participants